Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

copy: add a --all/-a flag #741

Merged
merged 1 commit into from
Oct 22, 2019
Merged

copy: add a --all/-a flag #741

merged 1 commit into from
Oct 22, 2019

Conversation

nalind
Copy link
Member

@nalind nalind commented Oct 14, 2019

Add a --all/-a flag to instruct us to attempt to copy all of the instances in the source image, if the source image specified to skopeo copy is actually a list of images. Previously, we'd just try to locate one for our preferred OS/arch combination.

This vendors the branch from containers/image#400 and a temporary workaround for containers/podman#4227, so it is not ready for merging in its current form.

@nalind nalind mentioned this pull request Oct 14, 2019
Closed
@nalind nalind force-pushed the manifest-lists branch 2 times, most recently from af802fb to dd291ab Compare October 14, 2019 22:06
vrothberg
vrothberg reviewed Oct 16, 2019
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to have some more test to hit more code paths. We could, for instance, copy docker://alpine:latest to oci:, dir:, and containers-storage: ... storage is currently erroring out.

@nalind
Copy link
Member Author

nalind commented Oct 16, 2019

I'd love to have some more test to hit more code paths. We could, for instance, copy docker://alpine:latest to oci:, dir:, and containers-storage: ... storage is currently erroring out.

Not sure I follow here. Are there paths that you have in mind that TestCopyWithManifestListConverge and TestCopyAllWithManifestListStorageFails (in integration/copy_test.go) in don't exercise?

@nalind
Copy link
Member Author

nalind commented Oct 16, 2019

Rebased.

@vrothberg
Copy link
Member

Not sure I follow here. Are there paths that you have in mind that TestCopyWithManifestListConverge and TestCopyAllWithManifestListStorageFails (in integration/copy_test.go) in don't exercise?

Eeek ... I assumed that storage would support it.

@vrothberg
Copy link
Member

Test are looking good and exhaustive 👍 I assumed that storage would support it with the c/image PR which made me conclude the tests aren't catching it.

@nalind nalind force-pushed the manifest-lists branch 3 times, most recently from f9b9bf5 to 464f541 Compare October 18, 2019 19:16
@nalind nalind mentioned this pull request Oct 21, 2019
Merged
@vrothberg
Copy link
Member

@nalind @rhatdan, we need this PR merged before cutting a new c/image release. I don't feel comfortable releasing c/image with a red CI.

@nalind
Copy link
Member Author

nalind commented Oct 22, 2019

Retargeted to pull in containers/image master.

@nalind nalind marked this pull request as ready for review October 22, 2019 14:55
@nalind nalind changed the title WIP: copy: add a --all/-a flag copy: add a --all/-a flag Oct 22, 2019
@vrothberg
Copy link
Member

@nalind, can you squash the commits? It'll reduce some noise when bisecting.

@nalind
copy: add a --all/-a flag
7d251f5 
Add a --all/-a flag to instruct us to attempt to copy all of the
instances in the source image, if the source image specified to "skopeo
copy" is actually a list of images.  Previously, we'd just try to locate
one for our preferred OS/arch combination.

Add a couple of tests to verify that we can copy an image into and then
back out of containers-storage.  The contents of an image that has been
copied out of containers-storage need a bit of tweaking to compensate
for containers-storage's habit of returning uncompressed versions of the
layer blobs that were originally written to it, in order to be
comparable to the image as it was when it was pulled from a registry.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@nalind
Copy link
Member Author

nalind commented Oct 22, 2019

Okay, squashed it all down.

vrothberg
vrothberg reviewed Oct 22, 2019
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merging so we unblock c/image CI 👍

@vrothberg vrothberg merged commit 4b6a5da into containers:master Oct 22, 2019
@nalind nalind deleted the manifest-lists branch October 30, 2019 18:33
@mtrmac mtrmac mentioned this pull request Feb 5, 2020
Closed
g-chauvel pushed a commit to g-chauvel/zuul-jobs that referenced this pull request May 15, 2020
@emonty
Add --all to skopeo copy from insecure registry
5f31625 
skopeo needs to be told to copy all instances of a given image,
otherwise it just grabs one of them.

containers/skopeo#741

Change-Id: If78ad50602e745ef7747d983b987cf964ff6e67f
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

Assignees
No one assigned
Labels
locked - please file new issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nalind @vrothberg